Skip to content

Fix a bunch of cases where default features were incorrectly included#2012

Open
cqundefine wants to merge 1 commit into
microsoft:mainfrom
cqundefine:default_features_fixes
Open

Fix a bunch of cases where default features were incorrectly included#2012
cqundefine wants to merge 1 commit into
microsoft:mainfrom
cqundefine:default_features_fixes

Conversation

@cqundefine
Copy link
Copy Markdown
Contributor

In some cases related to host dependencies, default features were being included even those they were disabled, found this when cross compiling qtbase with default features disabled.

Copilot AI review requested due to automatic review settings May 8, 2026 11:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes cases where default features were being incorrectly included, particularly around host dependencies (notably when cross-compiling with default features disabled). It updates dependency planning behavior and adds regression tests to ensure host/target default-feature metadata is handled as intended.

Changes:

  • Prevents adding a default-feature reinstall requirement for non-user-requested clusters when the package isn’t installed.
  • Adjusts versioned dependency graph handling so host dependencies don’t get implicit default features; default features for versioned plans are only emitted when enabled for the node.
  • Adds new unit tests covering host dependency default-feature suppression in both classic and versioned planning.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/vcpkg/dependencies.cpp Fixes default-feature inclusion logic in both classic cluster reinstall handling and versioned dependency planning (esp. host deps).
src/vcpkg-test/plan.cpp Adds tests asserting host-dependency default-feature metadata is suppressed appropriately in classic plans.
src/vcpkg-test/dependencies.cpp Adds a versioned-plan test ensuring host dependencies respect explicit no-defaults.

Comment thread src/vcpkg-test/plan.cpp Outdated
@cqundefine cqundefine force-pushed the default_features_fixes branch from f3f6d11 to f272979 Compare May 8, 2026 11:54
Copilot AI review requested due to automatic review settings May 8, 2026 12:04
@cqundefine cqundefine force-pushed the default_features_fixes branch from f272979 to c9b5533 Compare May 8, 2026 12:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/vcpkg/dependencies.cpp
@cqundefine cqundefine force-pushed the default_features_fixes branch from c9b5533 to a07cfe0 Compare May 8, 2026 13:51
@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

I would like to see the exact install command and problem before the change.

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

(For such cases, I prefer to push tests first, to demonstrate the error in CI.)

@cqundefine
Copy link
Copy Markdown
Contributor Author

cqundefine commented May 8, 2026

I would like to see the exact install command and problem before the change.

Right, so with this simple test package (just added it to the vcpkg repository for testing):

{
  "name": "testport",
  "version": "0.0.0",
  "description": "test",
  "dependencies": [
    {
      "name": "testport",
      "host": true,
      "default-features": false
    }
  ],
  "default-features": [
    "a"
  ],
  "features": {
    "a": {
      "description": "a"
    }
  }
}

Calling ./vcpkg install testport[core] --triplet=arm64-linux it incorrectly included the a feature for the host build (in my case x64-linux:

Computing installation plan...
The following packages will be built and installed:
  * testport[a,core]:[email protected]
    testport:[email protected]

Running the same command with a binary built with those fixes it correctly skips adding the a feature:

Computing installation plan...
The following packages will be built and installed:
  * testport:[email protected]
    testport:[email protected]

(For such cases, I prefer to push tests first, to demonstrate the error in CI.)

Do you think that is something I should do now, like revert the fix, let the CI run and then readd the fix?

@Osyotr
Copy link
Copy Markdown
Contributor

Osyotr commented May 8, 2026

Calling ./vcpkg install testport[core] --triplet=arm64-linux it incorrectly included the a feature for the host build (in my case x64-linux

This is not nice but expected, should've been ./vcpkg install testport[core] --triplet=arm64-linux testport[core]:x64-linux.
#177

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

Right, already works as intended by Microsoft. Cf. microsoft/vcpkg#51325 for a recent discussion.
TLDNR: If you don't want default-features for a particular package:triplet, be explicit in your install command.

@Thomas1664
Copy link
Copy Markdown
Contributor

Thomas1664 commented May 11, 2026

Right, already works as intended by Microsoft. Cf. microsoft/vcpkg#51325 for a recent discussion. TLDNR: If you don't want default-features for a particular package:triplet, be explicit in your install command.

I really don't understand why this should be the "intended" behavior: If I don't want default features and the port doesn't need them, why would vcpkg decide otherwise for the host triplet? if a port depends on itself on the host triplet, I think disabling default features on the host as well doesn't break anything because the user clearly said "I don't want default features for this port". This is fundamentally different from saying "I do want feature X on the target triplet". IMO, disabling default features should extend to the host triplet, given the number of complaints about this.

However, I do see that this would in general add the inconsistency of mirroring user settings between triplets. If I depend on a feature of port A, and port A has a dependency on itself, that feature would not be automatically added to the host triplet, so it makes sense that the same is true for requesting default features:

This is not nice but expected, should've been ./vcpkg install testport[core] --triplet=arm64-linux testport[core]:x64-linux.

@BillyONeal
Copy link
Copy Markdown
Member

The current behavior is not an "accident", see #177 which proposes a similar design change as described in the PR description.

It's true that default-features and host dependencies were designed in isolation and have created a situation that people seem unhappy with, but most of the heartburn there also applies to default features with non host dependencies.

If I don't want default features and the port doesn't need them, why would vcpkg decide otherwise for the host triplet?

If a port is not directly named by a user at the top level, it gets default features turned on. That is, given ports libarchive and B, feature libarchive[sevenzip], and default feature libarchive[zip], where B depends on libarchive[core] (also known as "default-features": false)

vcpkg install B

installs libarchive[zip] and B, even though neither B nor the command line asked for libarchive[zip].

The intended use case for default features are for cases like this. In libarchive, features are pluggable backends for particular archive formats. It's reasonable for a downstream component to want to be able to extract archives, but not depend on particular backends. However, a libarchive with no backends is a mistake; there's no point in an archive handling library with no archive handling formats selected. The expectation is that if a customer never names a component that the top level, that they don't "know" of it to explicitly ask to configure it. Therefore, to get no default features, both all dependency edges to the port need to say "default-features": false, and it must be named at the top level (command line or project vcpkg.json) with [core]/"default-features":false.

The problem is that users expect and use default features as merely "if I didn't say features on the command line these are what I meant," rather than the libarchive example above. They want a command line convenience where vcpkg install foo gives the most common features for foo, not features without which foo is effectively "broken". I think it's reasonable for users to want this, but that is not what the default-features feature does. This is part of why we have been discouraging adding new ports with default-features for some time, and why we have https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#default-features-should-enable-behaviors-not-apis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants